Skip to content

[Upstream] Import OCMock as a pod for RNTester on iOS#36239

Closed
Saadnajmi wants to merge 1 commit into
facebook:mainfrom
Saadnajmi:ocmock
Closed

[Upstream] Import OCMock as a pod for RNTester on iOS#36239
Saadnajmi wants to merge 1 commit into
facebook:mainfrom
Saadnajmi:ocmock

Conversation

@Saadnajmi
Copy link
Copy Markdown
Contributor

@Saadnajmi Saadnajmi commented Feb 22, 2023 •
edited
Loading

Summary

This is a change we have implemented in React Native macOS that I am now upstreaming: microsoft#1257

The library ocMock is currently imported as a checked in binary by RN-Tester to help run unit tests on iOS. That binary is only compiled for x86 on macOS, which meant we could not run tests on M1 / M2 Macs. Switching it so that we import from source as a pod should make maintenance easier for both iOS and macOS! :)

Original change notes:

Previously, we've been unable to test RNTester for macOS on an M1 machine. This is because we were using a framework that was prebuilt for Intel architecture, so the test components would fail to build.

The fix is to build OCMock from source directly instead of using a prebuilt version.

This is only necessary on macOS. The iOS version is already built for ARM architecture, as iOS devices have been running exclusively on ARM for a while now.

Changelog

[INTERNAL] [CHANGED] - Import OCMock as a pod

Test Plan

CI should pass as it did before.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner labels Feb 22, 2023
@analysis-bot
Copy link
Copy Markdown

analysis-bot commented Feb 22, 2023 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,477,095 +0
android hermes armeabi-v7a 7,799,079 +0
android hermes x86 8,953,089 +0
android hermes x86_64 8,810,587 +0
android jsc arm64-v8a 9,108,176 +0
android jsc armeabi-v7a 8,304,844 +0
android jsc x86 9,159,032 +0
android jsc x86_64 9,418,319 +0

Base commit: a49446b
Branch: main

@Saadnajmi Saadnajmi marked this pull request as ready for review February 22, 2023 17:00
cipolleschi
cipolleschi requested changes Mar 1, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Saadnajmi, could you please resolve the conflicts on the Podfile.lock?
Also, please revert the changes to the RNTesterPods.xcodeproj/project.pbxproj as those changes should be applied by the cocoapod script and should not be checked in.

I'll then import the PR as it looks like that everything is green and it allow us to remove a bunch of files (which translates to less maintenance burden... so that's really good!)

Thank you so much for doing this!

@Saadnajmi
Copy link
Copy Markdown
Contributor Author

Saadnajmi commented Mar 1, 2023

Hi @Saadnajmi, could you please resolve the conflicts on the Podfile.lock? Also, please revert the changes to the RNTesterPods.xcodeproj/project.pbxproj as those changes should be applied by the cocoapod script and should not be checked in.

I'll then import the PR as it looks like that everything is green and it allow us to remove a bunch of files (which translates to less maintenance burden... so that's really good!)

Thank you so much for doing this!

Thanks for the catch! I didn't notice my pbxproj changes were unrelated, oof. The latest iteration should be clean :). Also thanks also goes to @amgleitman who did this change originally on our fork.

cipolleschi
cipolleschi approved these changes Mar 2, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for polishing this up

@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Mar 2, 2023

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Mar 6, 2023
@facebook-github-bot
Copy link
Copy Markdown
Contributor

facebook-github-bot commented Mar 6, 2023

@cipolleschi merged this pull request in d00c150.

@Saadnajmi Saadnajmi deleted the ocmock branch April 6, 2023 23:28
@Saadnajmi Saadnajmi mentioned this pull request Sep 22, 2023
Closed
facebook-github-bot pushed a commit that referenced this pull request Sep 25, 2023
@Saadnajmi @facebook-github-bot
Remove undefined references to OCMock from RNTesters' pbxproj (#39616)
c62d174 
Summary:
In #36239 , I removed the copy of libOCMock we had locally in favor of a Pod. The references were left in RNTester's pbxproj and undefined. Let's just remove them.

## Changelog:

[INTERNAL] [FIXED] - Remove undefined references to OCMock from RNTesters' pbxproj

Pull Request resolved: #39616

Test Plan: CI should pass

Reviewed By: NickGerleman

Differential Revision: D49612102

Pulled By: ryancat

fbshipit-source-id: 85a5a67612dc58d5dba906edc1c56091d22b0978
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cipolleschi cipolleschi cipolleschi approved these changes

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Saadnajmi @analysis-bot @facebook-github-bot @cipolleschi